[core-amqp][event-hubs] add support for NamedKeyCredential and SASCredential #14423
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
chradek
commented
Mar 23, 2021
chradek
commented
Mar 23, 2021
chradek
requested review from
bterlson,
HarshaNalluru,
ramya-rao-a,
richardpark-msft and
xirzec
as code owners
chradek
commented
Mar 25, 2021
chradek
commented
Mar 25, 2021
| @@ -39,3 +39,4 @@ export { | |||
| } from "./util/utils"; | |||
| export { AmqpAnnotatedMessage } from "./amqpAnnotatedMessage"; | |||
| export { logger } from "./log"; | |||
| export * from "./internals"; | |||
There was a problem hiding this comment.
I really wanted to do export * as Internals from "./internals"; but unfortunately api-extractor doesn't support it (yet).
microsoft/rushstack#2412
|
/azp run js - event-hubs - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
chradek
changed the title
chradek
force-pushed
the
amqp-named-key-cred
branch
from
d9026c7
to
430b188
Compare
|
/azp run js - event-hubs - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ramya-rao-a
reviewed
Mar 26, 2021
ramya-rao-a
reviewed
Mar 26, 2021
…nProvider into its final form: SasTokenProvider!
chradek
force-pushed
the
amqp-named-key-cred
branch
from
430b188
to
30202e8
Compare
|
/azp run js - event-hubs - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ramya-rao-a
reviewed
Mar 27, 2021
|
Updated based on feedback! |
ramya-rao-a
approved these changes
Mar 29, 2021
|
/check-enforcer reset |
|
Hello @chradek! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit
jay-most
pushed a commit
to jay-most/azure-sdk-for-js
that referenced
this pull request
Apr 26, 2021
…dential (Azure#14423) Adds support for `NamedKeyCredential` and `SASCredential` to `@azure/event-hubs`. ## Background Event Hubs supports using token-based AAD auth via the `@azure/identity` credentials, and both "shared access key" and "shared access signature" via the connection string. This change allows the `EventHubConsumerClient` and `EventHubProducerClient` to support both "shared access key" and "shared access signature" methods of auth via credential objects defined by `@azure/core-auth`. ## It already supports connection strings, so why add 2 more credential types? Both the `AzureNamedKeyCredential` and `AzureSASCredential` classes support rotation by calling their respective `update` methods. Since connection strings can't be rotated within a client, the user would need to close their existing clients (which may be in the process of sending or receiving events!) and create new ones if their keys were rotated. With this change, the user only needs to call `update` on the credential they passed to their Event Hubs client, and the client will automatically use the new values. ## Why is this change in core-amqp as well? Service Bus and Event Hubs both need these changes. Note that the APIs added to core-amqp have the `@hidden` tag so they won't show up in documentation. ## TODO - [x] verify that the SDK retries 'unauthorized' errors. This, and the token expiry, will determine when the clients attempt to use the rotated credentials.
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds support for
NamedKeyCredentialandSASCredentialto@azure/event-hubs.Background
Event Hubs supports using token-based AAD auth via the
@azure/identitycredentials, and both "shared access key" and "shared access signature" via the connection string.This change allows the
EventHubConsumerClientandEventHubProducerClientto support both "shared access key" and "shared access signature" methods of auth via credential objects defined by@azure/core-auth.It already supports connection strings, so why add 2 more credential types?
Both the
AzureNamedKeyCredentialandAzureSASCredentialclasses support rotation by calling their respectiveupdatemethods. Since connection strings can't be rotated within a client, the user would need to close their existing clients (which may be in the process of sending or receiving events!) and create new ones if their keys were rotated.With this change, the user only needs to call
updateon the credential they passed to their Event Hubs client, and the client will automatically use the new values.Why is this change in core-amqp as well?
Service Bus and Event Hubs both need these changes. Note that the APIs added to core-amqp have the
@hiddentag so they won't show up in documentation.TODO